Skip to content

Comments

src: change v8:ConstructorBehavior in stream_base-inl.h#23453

Closed
bmacnaughton wants to merge 4 commits intonodejs:masterfrom
bmacnaughton:master
Closed

src: change v8:ConstructorBehavior in stream_base-inl.h#23453
bmacnaughton wants to merge 4 commits intonodejs:masterfrom
bmacnaughton:master

Conversation

@bmacnaughton
Copy link
Contributor

@bmacnaughton bmacnaughton commented Oct 12, 2018

Throw if addMethod functions used as constructors.
Implemented TimothyGu) TODO: None of these should
have ConstructorBehavior::kAllow

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@bmacnaughton
src: change v8:ConstructorBehavior in stream_base-inl.h
b838baf 
Throw if addMethod functions used as constructors.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 12, 2018
@gireeshpunathil gireeshpunathil added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 12, 2018
@addaleax
Copy link
Member

addaleax commented Oct 12, 2018

@bmacnaughton This PR is very similar to #23447 – that’s my fault. One of the tasks we handed out had text pointing to the same file, even though the heading pointed to another file (node_crypto.cc).

I’ll try to land both of these PRs together so you both get full credit, but just a heads up, if you have the test with the heading mentioning node_crypto.cc, feel free to also do that! Sorry again. :(

@addaleax addaleax mentioned this pull request Oct 12, 2018
Closed
4 tasks
@bmacnaughton
src: throw if functions used as constructors in node_crypto.cc
f8087e6 
Throw an error if verify_error_getter_templ or
verify_error_getter_templ2 are used as constructors.
addaleax
addaleax approved these changes Oct 12, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

@bmacnaughton
Copy link
Contributor Author

bmacnaughton commented Oct 12, 2018

I made the changes to node_crypto.cc as you noticed but didn't really think they belonged in the same branch because the message was wrong. So I reverted them, created a new branch, then resubmitted them.

I'm sorry for the extra things to look at. I appreciate you taking a quick look on this.

@addaleax
Copy link
Member

addaleax commented Oct 12, 2018

@bmacnaughton Don’t worry, we can make any of this work the way you want to. :) I’ll just assign these PRs to myself so nothing goes wrong while landing them :)

@addaleax addaleax self-assigned this Oct 12, 2018
@Trott
Copy link
Member

Trott commented Oct 14, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/17840/

@addaleax
Copy link
Member

addaleax commented Oct 14, 2018

Landed in 20b3698 and c19ab56, respectively – sorry again for the confusion!

@addaleax addaleax closed this Oct 14, 2018
addaleax pushed a commit that referenced this pull request Oct 14, 2018
@iansu @bmacnaughton @addaleax
src: change constructor behavior in stream_base-inl.h
c19ab56 
Change ConstructorBehavior from kAllow to kThrow.

Co-authored-by: Bruce A. MacNaughton <bmacnaughton@gmail.com>
Refs: #23453
PR-URL: #23447
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
@iansu @bmacnaughton @jasnell
src: change constructor behavior in stream_base-inl.h
ec7375a 
Change ConstructorBehavior from kAllow to kThrow.

Co-authored-by: Bruce A. MacNaughton <bmacnaughton@gmail.com>
Refs: #23453
PR-URL: #23447
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

+1 more reviewer

@TimothyGu TimothyGu TimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@addaleax addaleax

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@bmacnaughton @addaleax @Trott @jasnell @TimothyGu @cjihrig @gireeshpunathil @nodejs-github-bot